Skip to content

Port BrandedArray fix for Array-as-parent bug in destroyables (glimmer-vm#1771)#21139

Merged
ef4 merged 2 commits intomainfrom
copilot/port-bugfix-destroyable-meta
Mar 3, 2026
Merged

Port BrandedArray fix for Array-as-parent bug in destroyables (glimmer-vm#1771)#21139
ef4 merged 2 commits intomainfrom
copilot/port-bugfix-destroyable-meta

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 3, 2026 •
edited
Loading

DestroyableMeta used Array.isArray() to distinguish single items from multi-item OneOrMany<T> collections, making it impossible to use a plain array as a parent in associateDestroyableChild — the array was misidentified as an internal multi-item collection, breaking push, iterate, and remove.

Changes

  • packages/@glimmer/destroyable/index.ts

    • OneOrMany<T> now uses BrandedArray<T> instead of T[] for the "many" case
    • Added branded symbol, BrandedArray<T> type, and isBrandedArray() type guard
    • push brands newly-created multi-item arrays; iterate and remove use isBrandedArray() instead of Array.isArray()

  • packages/@glimmer/destroyable/test/destroyables-test.ts

    • Added 'parent can be an array' test covering the previously broken case
// Previously broken — array parent was misidentified as a multi-item collection
const parent: unknown[] = ['a'];
const child = {};
associateDestroyableChild(parent, child); // would malfunction
registerDestructor(child, () => assert.step('child'));
destroy(child);
flush();
assert.verifySteps(['child']); // now passes correctly
Original prompt

Port the bugfix from glimmerjs/glimmer-vm#1771 (by @ef4) to emberjs/ember.js.

Problem

The DestroyableMeta is trying to be clever about avoiding extra allocations, so it uses the OneOrMany type a lot. But this means it cannot accurately distinguish between the case where your parent is an array vs the case where you have multiple parents. When an Array is used as a parent in associateDestroyableChild, the internal push, iterate, and remove helpers incorrectly treat it as the "many" case of OneOrMany instead of as a single parent that happens to be an array.

Solution

Add a Symbol brand to the internally-created arrays that represent multiple parents/children so they are distinguishable from a single parent/child that happens to be an array.

Changes needed

File 1: packages/@glimmer/destroyable/index.ts

The current code at the top of the file has:

type OneOrMany<T> = null | T | T[];

This needs to change to:

type OneOrMany<T> = null | T | BrandedArray<T>;

After the DESTROYABLE_META declaration (after line 28), add the branded array symbol, type, and type guard:

const branded = Symbol('BrandedArray');
type BrandedArray<T> = T[] & { [branded]: true };

function isBrandedArray<T>(collection: OneOrMany<T>): collection is BrandedArray<T> {
  return Array.isArray(collection) && branded in collection;
}

In the push function, change Array.isArray(collection) to isBrandedArray(collection), and change the else branch from return [collection, newItem] to:

    const b = [collection, newItem] as BrandedArray<T>;
    b[branded] = true;
    return b;

In the iterate function, change Array.isArray(collection) to isBrandedArray(collection).

In the remove function, change both instances of Array.isArray(collection) to isBrandedArray(collection).

File 2: packages/@glimmer/destroyable/test/destroyables-test.ts

Add a new test case after the "destroying child before a parent works" test (after line 236) and before the "children can have multiple parents, but only destroy once" test:

  test('parent can be an array', (assert) => {
    const parent: unknown[] = ['a'];
    const child = {};
    associateDestroyableChild(parent, child);
    registerDestructor(child, () => assert.step('child'));
    destroy(child);
    flush();
    assert.verifySteps(['child'], 'child destructor run');
  });

Important Notes

This pull request was created from Copilot chat.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Port bugfix from glimmerjs/glimmer-vm#1771 to emberjs/ember.js Port BrandedArray fix for Array-as-parent bug in destroyables (glimmer-vm#1771) Mar 3, 2026
@NullVoxPopuli
Copy link
Copy Markdown
Contributor

NullVoxPopuli commented Mar 3, 2026

Compare with: glimmerjs/glimmer-vm#1771

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review March 3, 2026 16:26
Copilot AI and others added 2 commits March 3, 2026 11:45
@NullVoxPopuli
Initial plan
58d45b6
@NullVoxPopuli
Port BrandedArray bugfix from glimmer-vm#1771 to fix Array-as-parent …
5859b78 
…bug in destroyables

Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
@NullVoxPopuli NullVoxPopuli force-pushed the copilot/port-bugfix-destroyable-meta branch from 16e09a4 to 5859b78 Compare March 3, 2026 16:45
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 3, 2026

Estimated Asset Sizes

Diff

--- main/out.txt	2026-03-03 16:45:40.000000000 +0000
+++ pr/./pr-22633253461/out.txt	2026-03-03 16:46:26.000000000 +0000
@@ -1,7 +1,7 @@
 ╔═══════╤═══════════╤═══════════╗
 ║       │ Min       │ Gzip      ║
 ╟───────┼───────────┼───────────╢
-║ Total │ 351.99 KB │ 203.84 KB ║
+║ Total │ 352.16 KB │ 203.97 KB ║
 ╚═══════╧═══════════╧═══════════╝
 
 ╔══════════════════════╤═══════════╤═══════════╗
@@ -40,9 +40,9 @@
 ╔═════════════════╤══════════╤══════════╗
 ║ @glimmer/*      │ Min      │ Gzip     ║
 ╟─────────────────┼──────────┼──────────╢
-║ Total           │ 38.6 KB  │ 21.94 KB ║
+║ Total           │ 38.77 KB │ 22.06 KB ║
 ╟─────────────────┼──────────┼──────────╢
-║ destroyable     │ 2.77 KB  │ 1.39 KB  ║
+║ destroyable     │ 2.95 KB  │ 1.52 KB  ║
 ║ encoder         │ 81 B     │ 171 B    ║
 ║ env             │ 38 B     │ 87 B     ║
 ║ global-context  │ 886 B    │ 545 B    ║

Details

This PRmain
 
╔═══════╤═══════════╤═══════════╗
║       │ Min       │ Gzip      ║
╟───────┼───────────┼───────────╢
║ Total │ 352.16 KB │ 203.97 KB ║
╚═══════╧═══════════╧═══════════╝

╔══════════════════════╤═══════════╤═══════════╗
║ @ember/*             │ Min       │ Gzip      ║
╟──────────────────────┼───────────┼───────────╢
║ Total                │ 313.39 KB │ 181.91 KB ║
╟──────────────────────┼───────────┼───────────╢
║ -internals           │ 36.65 KB  │ 26.22 KB  ║
║ application          │ 13.23 KB  │ 8.05 KB   ║
║ array                │ 13.01 KB  │ 7.46 KB   ║
║ canary-features      │ 304 B     │ 389 B     ║
║ component            │ 2.05 KB   │ 1.64 KB   ║
║ controller           │ 1.96 KB   │ 1.41 KB   ║
║ debug                │ 11.69 KB  │ 8.12 KB   ║
║ deprecated-features  │ 31 B      │ 77 B      ║
║ destroyable          │ 561 B     │ 383 B     ║
║ enumerable           │ 259 B     │ 387 B     ║
║ helper               │ 1.08 KB   │ 811 B     ║
║ instrumentation      │ 2.43 KB   │ 1.79 KB   ║
║ modifier             │ 1.22 KB   │ 965 B     ║
║ object               │ 35.94 KB  │ 22.16 KB  ║
║ owner                │ 159 B     │ 178 B     ║
║ renderer             │ 630 B     │ 487 B     ║
║ routing              │ 59.3 KB   │ 34.12 KB  ║
║ runloop              │ 2.36 KB   │ 1.5 KB    ║
║ service              │ 1 KB      │ 845 B     ║
║ template             │ 654 B     │ 541 B     ║
║ template-compilation │ 429 B     │ 366 B     ║
║ template-compiler    │ 123.08 KB │ 59.45 KB  ║
║ template-factory     │ 370 B     │ 374 B     ║
║ test                 │ 923 B     │ 627 B     ║
║ utils                │ 4.11 KB   │ 3.6 KB    ║
║ version              │ 55 B      │ 131 B     ║
╚══════════════════════╧═══════════╧═══════════╝

╔═════════════════╤══════════╤══════════╗
║ @glimmer/*      │ Min      │ Gzip     ║
╟─────────────────┼──────────┼──────────╢
║ Total           │ 38.77 KB │ 22.06 KB ║
╟─────────────────┼──────────┼──────────╢
║ destroyable     │ 2.95 KB  │ 1.52 KB  ║
║ encoder         │ 81 B     │ 171 B    ║
║ env             │ 38 B     │ 87 B     ║
║ global-context  │ 886 B    │ 545 B    ║
║ manager         │ 977 B    │ 608 B    ║
║ node            │ 175 B    │ 260 B    ║
║ opcode-compiler │ 1.11 KB  │ 894 B    ║
║ owner           │ 159 B    │ 202 B    ║
║ program         │ 252 B    │ 301 B    ║
║ reference       │ 548 B    │ 531 B    ║
║ runtime         │ 10.32 KB │ 5.32 KB  ║
║ tracking        │ 1.34 KB  │ 1.16 KB  ║
║ util            │ 1.94 KB  │ 1.68 KB  ║
║ validator       │ 15.75 KB │ 6.96 KB  ║
║ vm              │ 495 B    │ 569 B    ║
║ wire-format     │ 1.84 KB  │ 1.35 KB  ║
╚═════════════════╧══════════╧══════════╝

╔═══════╤═══════════╤═══════════╗
║       │ Min       │ Gzip      ║
╟───────┼───────────┼───────────╢
║ Total │ 351.99 KB │ 203.84 KB ║
╚═══════╧═══════════╧═══════════╝

╔══════════════════════╤═══════════╤═══════════╗
║ @ember/*             │ Min       │ Gzip      ║
╟──────────────────────┼───────────┼───────────╢
║ Total                │ 313.39 KB │ 181.91 KB ║
╟──────────────────────┼───────────┼───────────╢
║ -internals           │ 36.65 KB  │ 26.22 KB  ║
║ application          │ 13.23 KB  │ 8.05 KB   ║
║ array                │ 13.01 KB  │ 7.46 KB   ║
║ canary-features      │ 304 B     │ 389 B     ║
║ component            │ 2.05 KB   │ 1.64 KB   ║
║ controller           │ 1.96 KB   │ 1.41 KB   ║
║ debug                │ 11.69 KB  │ 8.12 KB   ║
║ deprecated-features  │ 31 B      │ 77 B      ║
║ destroyable          │ 561 B     │ 383 B     ║
║ enumerable           │ 259 B     │ 387 B     ║
║ helper               │ 1.08 KB   │ 811 B     ║
║ instrumentation      │ 2.43 KB   │ 1.79 KB   ║
║ modifier             │ 1.22 KB   │ 965 B     ║
║ object               │ 35.94 KB  │ 22.16 KB  ║
║ owner                │ 159 B     │ 178 B     ║
║ renderer             │ 630 B     │ 487 B     ║
║ routing              │ 59.3 KB   │ 34.12 KB  ║
║ runloop              │ 2.36 KB   │ 1.5 KB    ║
║ service              │ 1 KB      │ 845 B     ║
║ template             │ 654 B     │ 541 B     ║
║ template-compilation │ 429 B     │ 366 B     ║
║ template-compiler    │ 123.08 KB │ 59.45 KB  ║
║ template-factory     │ 370 B     │ 374 B     ║
║ test                 │ 923 B     │ 627 B     ║
║ utils                │ 4.11 KB   │ 3.6 KB    ║
║ version              │ 55 B      │ 131 B     ║
╚══════════════════════╧═══════════╧═══════════╝

╔═════════════════╤══════════╤══════════╗
║ @glimmer/*      │ Min      │ Gzip     ║
╟─────────────────┼──────────┼──────────╢
║ Total           │ 38.6 KB  │ 21.94 KB ║
╟─────────────────┼──────────┼──────────╢
║ destroyable     │ 2.77 KB  │ 1.39 KB  ║
║ encoder         │ 81 B     │ 171 B    ║
║ env             │ 38 B     │ 87 B     ║
║ global-context  │ 886 B    │ 545 B    ║
║ manager         │ 977 B    │ 608 B    ║
║ node            │ 175 B    │ 260 B    ║
║ opcode-compiler │ 1.11 KB  │ 894 B    ║
║ owner           │ 159 B    │ 202 B    ║
║ program         │ 252 B    │ 301 B    ║
║ reference       │ 548 B    │ 531 B    ║
║ runtime         │ 10.32 KB │ 5.32 KB  ║
║ tracking        │ 1.34 KB  │ 1.16 KB  ║
║ util            │ 1.94 KB  │ 1.68 KB  ║
║ validator       │ 15.75 KB │ 6.96 KB  ║
║ vm              │ 495 B    │ 569 B    ║
║ wire-format     │ 1.84 KB  │ 1.35 KB  ║
╚═════════════════╧══════════╧══════════╝

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

NullVoxPopuli commented Mar 3, 2026

Perf:


duration phase no difference [-14ms to 19ms]
renderEnd phase no difference [-2ms to 3ms]
render1000Items1End phase no difference [0ms to 9ms]
clearItems1End phase no difference [-2ms to 2ms]
render1000Items2End phase no difference [-1ms to 0ms]
clearItems2End phase no difference [-8ms to 1ms]
render5000Items1End phase no difference [-2ms to 5ms]
clearManyItems1End phase no difference [-2ms to 2ms]
render5000Items2End phase no difference [-4ms to 7ms]
clearManyItems2End phase no difference [-1ms to 0ms]
render1000Items3End phase no difference [-1ms to 0ms]
append1000Items1End phase no difference [-3ms to 2ms]
append1000Items2End phase no difference [-8ms to 1ms]
updateEvery10thItem1End phase no difference [0ms to 1ms]
updateEvery10thItem2End phase no difference [0ms to 1ms]
selectFirstRow1End phase no difference [0ms to 0ms]
selectSecondRow1End phase no difference [-1ms to 0ms]
removeFirstRow1End phase no difference [-7ms to 0ms]
removeSecondRow1End phase no difference [0ms to 0ms]
swapRows1End phase no difference [-1ms to 0ms]
swapRows2End phase no difference [0ms to 0ms]
clearItems4End phase no difference [-1ms to 0ms]
paint phase no difference [0ms to 1ms]

@ef4 ef4 merged commit 2c5ff30 into main Mar 3, 2026
30 checks passed
@ef4 ef4 deleted the copilot/port-bugfix-destroyable-meta branch March 3, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ef4 ef4 ef4 approved these changes

+1 more reviewer

@NullVoxPopuli NullVoxPopuli NullVoxPopuli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@NullVoxPopuli NullVoxPopuli

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@NullVoxPopuli @ef4